Skip to content

fix: fix type-instability of get_root_indp #1074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AayushSabharwal
Copy link
Member

No description provided.

test/runtests.jl Outdated
@@ -60,6 +60,9 @@ end
@time @safetestset "Clocks" begin
include("clock.jl")
end
@time @safetestset "JET" begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to go into a QA session.

@RomeoV
Copy link

RomeoV commented Jul 31, 2025

Can this be merged? Similar to SciML/DiffEqBase.jl#1175

@ChrisRackauckas
Copy link
Member

No it breaks a lot of tests in its current state and needs to get fixed first

@RomeoV
Copy link

RomeoV commented Jul 31, 2025

Makes sense. Just noting that this currently stops trimming (or at least JET being happy), but I'm happy to just use this PR branch for now. If others are looking for an alternative dirty fix that also seems to work, I've had success by pirating get_root_indp like so:

import NonlinearSolve.SciMLBase.get_root_indp
NonlinearSolve.SciMLBase.get_root_indp(prob::NonlinearLeastSquaresProblem) = prob.p

@ChrisRackauckas
Copy link
Member

It's safe to use. We just have a lot of legacy tests to fix up with this. In particular, we used to have an old method for passing an array of symbols syms and there's a deprecation path etc., all an old undocumented thing that ParameterizedFunctions.jl hooked into, but kept around. It seems that the reason for the complexity here is the deprecation path, so it's fine to just remove it since nothing has used it for probably years now (and it was never user facing or documented), but we need to make sure we do that and update the tests otherwise we'll have breakage. So probably in the next week.

This week we did an entire CI overhaul so we're a bit behind 😅 but we should be moving faster now that everything should be clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants